Skip to content

[bgfx] [bimg] [bx] Add new port#10465

Closed
BurningEnlightenment wants to merge 6 commits intomicrosoft:masterfrom
BurningEnlightenment:dev/Enlightenment/bgfx
Closed

[bgfx] [bimg] [bx] Add new port#10465
BurningEnlightenment wants to merge 6 commits intomicrosoft:masterfrom
BurningEnlightenment:dev/Enlightenment/bgfx

Conversation

@BurningEnlightenment
Copy link
Copy Markdown
Contributor

Add a port for bgfx. As opposed to #9369 this mostly doesn't use vendored packages, because I replaced the GENIe buildsystem with a straight forward cmake buildsystem with the added benefit that cmake import targets can be provided for everything.

I rewrote the cmake files from scratch and didn't use the ones provided by JoshuaBrookover/bgfx.cmake in order to avoid potential licensing/CLA issues.

However, it would be absolutely awesome if @JoshuaBrookover could review the approach I've taken. Anyways, many thanks for maintaining bgfx.cmake!


The bimg and bgfx ports currently rely on some vendored packages which are currently not available in vcpkg. Also I needed to update some of the existing ports, obviously those updates need to be merged first:

The port has been tested with the x64-windows-static and x64-linux triplets. OSX support is largely lacking due to not having anything to test with.
A "preview" branch with all changes merged can be found here: bgfx-asm


Resolves #1303
Closes #9369

Build the amalgated version of the library using a custom CMake project
in order to provide cmake import configs.
Build the library using a custom CMake project in order to provide cmake
import configs.
Build the amalgated version of the library using a custom CMake project
in order to provide cmake import configs.

- Provide feature flags for the most common renderer backends.
- Provide feature flags for building the examples and/or tools.
@dan-shaw
Copy link
Copy Markdown
Contributor

/azp run

Comment thread ports/bgfx/portfile.cmake
@@ -0,0 +1,82 @@

vcpkg_fail_port_install(ON_TARGET "OSX" "UWP" "ANDROID" ON_LIBRARY_LINKAGE "dynamic")
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since ON_LIBRARY_LINKAGE "dynamic" is not suitable to be used here. ANDROID is not support in vcpkg now.
Please update this as
vcpkg_fail_port_install(ON_TARGET "OSX" "UWP").

If this port only supports static build, you can add vcpkg_check_linkage(ONLY_STATIC_LIBRARY).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this port only supports static build, you can add vcpkg_check_linkage(ONLY_STATIC_LIBRARY).

Well, the upstream project does support a shared build which exposes the c99 API which currently isn't implemented by this cmake project. Should we still simply fallback to static builds?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can update this port again if it is implemented by this cmake project.
Currently, we can only support static build.

Comment thread ports/bgfx/CONTROL Outdated
Description: Cross-platform, graphics API agnostic, "Bring Your Own Engine/Framework" style rendering library.
Build-Depends: bx
Default-Features: opengl, d3d11, tools
Supports: (windows|linux|static)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might be updated as Supports: !(uwp|osx).

Comment thread ports/bgfx/CONTROL Outdated
Comment thread ports/bgfx/CONTROL Outdated
Comment thread ports/bgfx/CONTROL Outdated
Comment thread ports/bgfx/portfile.cmake
# * glsl-optimizer -- https://github.com/aras-p/glsl-optimizer
# * iconfontheaders -- https://github.com/juliettef/IconFontCppHeaders
# * meshoptimizer -- https://github.com/zeux/meshoptimizer
# * sdf -- unknown origin
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can these comments be removed here?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know about the "no comments" guideline, however I'd rather let them stay, because they clearly identify potential port improvements and will help future maintainers implementing these.

Comment thread ports/bimg/CONTROL Outdated
Comment thread ports/bimg/portfile.cmake Outdated
Comment thread ports/bimg/portfile.cmake
# * etc2 -- unknown source; defines symbols like `::g_table`, high likelyhood of ODR violations
# * iqa -- https://sourceforge.net/projects/iqa or https://github.com/tjdistler/iqa
# * nvtt -- has a vcpkg port which needs to be thoroughly refactored (conflicts with libsquish and others)
# * pvrtc -- https://bitbucket.org/jthlim/pvrtccompressor/
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can these comments be removed here?

Comment thread ports/bx/portfile.cmake Outdated
@NancyLi1013
Copy link
Copy Markdown
Contributor

/azp run

Upstream bkaradzic/bgfx#2070 recommends using the vendored OpenGL/vulkan
headers, because the respective libraries are only dynamically linked at
runtime, i.e. ODR violations shouldn't be possible considering the
quite minimal C inteface exposed.

- Fix some stylistic nits.
- Correctly include transitive dependencies.
@BurningEnlightenment
Copy link
Copy Markdown
Contributor Author

BurningEnlightenment commented Mar 21, 2020

I'm thinking about generally using the vendored dependencies except for those cases where the dependencies are ODR used by installed/exported libraries like lodepng, tinyexr, stb, etc. This would allow us to drop the include patches for cgltf, freetype and glslang without any observable side effects. I will however ask again if the other patches can be integrated upstream

Comment thread ports/bgfx/portfile.cmake
@@ -0,0 +1,82 @@

vcpkg_fail_port_install(ON_TARGET "OSX" "UWP" "ANDROID" ON_LIBRARY_LINKAGE "dynamic")
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can update this port again if it is implemented by this cmake project.
Currently, we can only support static build.

@NancyLi1013
Copy link
Copy Markdown
Contributor

/azp run

@NancyLi1013
Copy link
Copy Markdown
Contributor

@BurningEnlightenment
Is work still being done for this PR?

@NancyLi1013
Copy link
Copy Markdown
Contributor

/azp run

@azure-pipelines
Copy link
Copy Markdown

Azure Pipelines failed to run 1 pipeline(s).

@NancyLi1013
Copy link
Copy Markdown
Contributor

@BurningEnlightenment

Thanks for the PR; we're closing this for now since there's been no response. If you'd like to continue working on it, please reopen and ping us!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Port request: bgfx

4 participants